-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Serialize only simple types to session in TempData #2317
Conversation
|
cc @Eilon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add test to verify that all the types mentioned in TypeHelper.IsSimpleType can be deserialized...I am specifically interested about struct and also not sure about Uri
https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNet.Mvc.Common/TypeHelper.cs#L68
|
We need functional tests that verify these values round trip correctly - if I throw in a |
|
Updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting nit: member data needs to preceed the actual test.
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to consult with @blowdart to use this because it is generally not allowed to use any TypeNameHandling value other than None. I don't know if we can guarantee that the data is coming from a trusted source. For example, suppose that the session state provider is a custom one that stores all session data in a cookie - that is no longer trusted data because it can be tampered. Our default session state providers can't be tampered because they are stored in trusted environments (local machine memory, Azure Cache, etc.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. You can't ship this. Use of Auto is banned because it enables RCE. Unless you can ensure every input is signed, and validated you need to find another way to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blowdart FYI - be happy 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When am I ever HAPPY? I am content.
|
What about dictionaries? Is it easy to support those? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you create a new dictionary rather than writing to this one (preferably one with StringComparer.OrdinalIgnoreCase semantics? Seems cleaner than doing a ToList here. An additional advantage is we could change the dictionary we wrap in TempDataDictionary to be a copy-on-write one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_dictionaryConverters
|
It can only be string because |
|
Good :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do this early on as part of detecting the IDictionary<,>. You can avoid having Type[] actualTypes and just have one.
|
⌚ |
|
Updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The '{0}' cannot serialize a dictionary with a key of type '{1}' to session state. (Swapped {0} and {1} too.)
|
Just some small string comments, then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just split this out:
if (itemType.ExtractGenericInterface(typeof(IList<>)))
{
Debug.Asser(itemType.GetGenericArguments().Length == 1, "IList<T> has one argument");
itemType = itemType.GetGenericArguments()[0];
}
if (itemType.ExtractGenericInterface(typeof(IDictionary<>)))
{
var genericTypeArguments = itemType.GetGenericArguments();
Debug.Assert(genericTypeArguments.Length == 0);
...
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make JToken a parameter.
|
|
Issue - #2276
@pranavkm